Wasn't sure how to declare these types. If I don't declare anything, my type checking complains anywhere I use _network_config as a dict because sources.UNSET is a string. I still get a complaint on this line because I'm declaring a string as a dict, but it has quieted the rest of the warnings. If we were being strict it would be a Union[dict|str], but since we don't ever actually want to use it as a string, that also seemed wrong to me (and I didn't want to add the union import just for that).

Shrug. Something to think about if we start to incorporate more typing.

Yeah, I think sources.UNSET is going to be a barrier to typing the codebase more generally, so I think we should set it to one side here. I'd probably leave this untyped, rather than applying a not-quite-correct annotation; we'll correct it as part of a broader change.

I don't love using suppress here, but duplicating the read_opc_metadata() call (especially if it's taking arguments now) seems worse to me.

Agreed; elsewhere in the codebase (albeit in testing code) we use ExitStack for this, and we import it as something that indicates how we're using it:

What do you think to following that pattern here? (Perhaps as noop is more apropos here? Or maybe nullcontext?)

Agreed; elsewhere in the codebase (albeit in testing code) we use ExitStack for this, and we import it as something that indicates how we're using it:

What do you think to following that pattern here? (Perhaps as noop is more apropos here? Or maybe nullcontext?)

Yeah, I think sources.UNSET is going to be a barrier to typing the codebase more generally, so I think we should set it to one side here. I'd probably leave this untyped, rather than applying a not-quite-correct annotation; we'll correct it as part of a broader change.

"Network config" is a little ambiguous in this context; I had to read this a couple of times to see that this was testing self._vnics_data, not self._network_config.

I'm not sure return None is the right behaviour here: _add_network_config_from_opc_imds only adds to self._network_config, so we do have some valid config already determined at this point. If we return None from .network_config then _find_networking_config in stages.py will move onto considering other sources of network config.

For Oracle, the order in which network config sources will be considered is:

So if we return None here: if there is initramfs configuration then that will be used, if not then fallback config will be generated (the system_cfg source is used rarely). That logic is identical to the logic here, but excludes the addition of secondary NICs (which is fine; we know there aren't any if we're executing this line) and the call to _ensure_netfailover_safe. The message on the commit that introduced _ensure_netfailover_safe (fa47d52) indicates that this should be called for fallback network configuration, so this is less fine.

(TL;DR: I think we need to skip the addition of the secondary NICs here, rather than aborting processing entirely.)

This isn't testing retries on the VNIC route. (I suspect a separate test which just takes the happy path for instance data so it can test VNIC fetching in isolation would be easiest.)

Having reviewed the whole PR now: I think we're missing fetch_vnics_data testing more generally: all the tests still pass with this patch applied:

--- a/cloudinit/sources/DataSourceOracle.py
+++ b/cloudinit/sources/DataSourceOracle.py
@@ -305,6 +305,7 @@ def read_opc_metadata(*, fetch_vnics_data: bool = False):
         )._response.json()
     vnics_data = None
     if fetch_vnics_data:
+        assert False
         try:
             vnics_data = readurl(
                 METADATA_PATTERN.format(

We now perform the defaulting of configure_secondary_nics in two places: here and in .network_config. It would be preferable to have the default configurable in a single place for ease of maintenance.

I'm don't understand what you mean by 'defaulting of configure_secondary_nics'. Can you explain it?

This line determines what the default value of configure_secondary_nics will be if it isn't present in self.ds_cfg: False. In .network_config, we also determine what the default value of configure_secondary_nics will be if it isn't present (and in that case as we don't supply a second argument to .get, it will be None).

Though, further confusing matters, we actually set the default in BUILTIN_DS_CONFIG which should mean that configure_secondary_nics is always set in self.ds_cfg (so we could use regular dict access instead of .get). However, that goes through util.mergemanydict and I don't know for sure that that couldn't result in a dict without the key present under certain user inputs.

Does that make any more sense?

Yes, mainly because of this

so we could use regular dict access instead of .get

I guess I don't see the problem with using .get here (or anywhere and everything 😉 ). Since we're not persisting the result of this anywhere, I don't see it as "defaulting a value", but rather eliminating the need for None checks in case something hasn't been set. Why would having things as they are increase the maintenance burden?

This is still written as if this callable takes a parameter.

Should we also be testing the version attribute of read_opc_metadata's return here (and in the test below)?

It looks like the package build in xenial is failing during the tests: https://travis-ci.com/github/canonical/cloud-init/jobs/371325182

I believe this is because the version of httpretty (0.8.6) in xenial does not include gabrielfalcao/HTTPretty@bdb7ee7 (which was first included in 0.8.11). So I think you'll need to add some default response header values to avoid these tracebacks.

(This isn't caught by the "xenial" unittests in Travis because we don't have HTTPretty pinned correctly; I'll be submitting a PR to address that shortly.)

#531 is that PR.

Don't love this continuation; black would format this as:

network_context = (
    noop()
    if _is_iscsi_root()
    else dhcp.EphemeralDHCPv4(net.find_fallback_nic())
)

which is actually more lines than:

if _is_iscsi_root():
    network_context = noop()
else:
    network_context = dhcp.EphemeralDHCPv4(net.find_fallback_nic())

and you can even golf that down a little further if you want:

network_context = noop()
if not _is_iscsi_root():
    network_context = dhcp.EphemeralDHCPv4(net.find_fallback_nic())

(And before you say anything:

In [2]: len("        network_context = noop() if _is_iscsi_root() else dhcp.EphemeralDHCPv4(net.find_fallback_nic())") > 100
Out[2]: True

😁 )

Now that I'm not being distracted by the type annotation, I've realised that I have this question: is there any reason to use sources.UNSET over None here? The use of this attribute is entirely contained to this class, so I don't think we need to follow that (IMO, anti-)pattern.

What do you think to moving this guard into _add_network_config_from_opc_imds? As currently written, _add_network_config_from_opc_imds will behave confusingly if called before self._vnics_data is set. It seems natural for that method to own its own input validation.

(As things stand, I think it will behave very confusingly: self._vnics_data will be "_unset", so we'll get a TypeError: string indices must be integers when we attempt to effectively do "u"['.macAddr'].lower(); if we do switch to self._vnics_data = None in __init__ then I think we'll get the less-confusing-but-still-avoidable: TypeError: 'NoneType' object is not subscriptable.)